Use musl/native version of atexit - #15905
Merged
Merged
Conversation
sbc100
force-pushed
the
atexit_no_proxy
branch
3 times, most recently
from
January 7, 2022 02:14
e89185d to
ee67165
Compare
sbc100
force-pushed
the
atexit_no_proxy
branch
2 times, most recently
from
January 7, 2022 11:49
7215d83 to
76e5833
Compare
kripken
reviewed
Jan 7, 2022
kripken
left a comment
Member
There was a problem hiding this comment.
Maybe the PR title could be "Move atexit() to C, and run them on each thread" or such?
Collaborator
Author
But the atexit function still only run on the main thread. I will update the title though. |
atexit
kripken
reviewed
Jan 7, 2022
sbc100
force-pushed
the
atexit_no_proxy
branch
5 times, most recently
from
January 7, 2022 19:01
7e6b02c to
129fe7e
Compare
kripken
approved these changes
Jan 7, 2022
kripken
left a comment
Member
There was a problem hiding this comment.
Ah, good idea to link in stubs for atexit. Yes, I think that's the right approach.
lgtm, but also please delete these 2 lines:
Lines 540 to 541 in 8988358
That pass removes calls to atexit as JS imports. Now that we have no more JS imports for them it does not do anything. We can remove it from Binaryen after this lands actually.
To achieve this we manage the `atexit` functions in native code using existing musl code. This is small step towards a large change to just use musl for all `atexit` handling: #14479. The codesize implications of this change are a mixed bag. In some places we see saving but in other cases the extra export causes a small regression (only when EXIT_RUNTIME=1). In the long, once we land #14479 there should be more code size saving to be had by doing everything on the native side. Fixes #15868
sbc100
force-pushed
the
atexit_no_proxy
branch
from
January 7, 2022 19:43
129fe7e to
217252a
Compare
sbc100
enabled auto-merge (squash)
January 7, 2022 20:18
sbc100
added a commit
to WebAssembly/binaryen
that referenced
this pull request
Jan 8, 2022
Since emscripten-core/emscripten#15905 landed emscripten now includes its own dummy atexit function when building with EXIT_RUNTIME=0. This dummy function conflicts with the emscripten-provided one: ``` wasm-ld: error: duplicate symbol: atexit >>> defined in CMakeFiles/binaryen_wasm.dir/src/binaryen-c.cpp.o >>> defined in ...wasm32-emscripten/lto/libnoexit.a(atexit_dummy.o) ``` Normally overriding symbols from libc does not causes issues but one needs to be sure to override all the symbols in a given object file so that the object in question (atexit_dummy.o) does not get linked in. In this case some other symbol being defined in in atexit_dummy.o (e.g. __cxa_atexit) is likely the cause of the conflict. Overriding symbols from libc is likely to break in this way as the libc evolves, and since emscripten is now providing a dummy, just as we want, its better/safer to simply remove our dummy.
sbc100
added a commit
to WebAssembly/binaryen
that referenced
this pull request
Jan 9, 2022
Since emscripten-core/emscripten#15905 landed emscripten now includes its own dummy atexit function when building with EXIT_RUNTIME=0. This dummy function conflicts with the emscripten-provided one: ``` wasm-ld: error: duplicate symbol: atexit >>> defined in CMakeFiles/binaryen_wasm.dir/src/binaryen-c.cpp.o >>> defined in ...wasm32-emscripten/lto/libnoexit.a(atexit_dummy.o) ``` Normally overriding symbols from libc does not causes issues but one needs to be sure to override all the symbols in a given object file so that the object in question (atexit_dummy.o) does not get linked in. In this case some other symbol being defined in in atexit_dummy.o (e.g. __cxa_atexit) is likely the cause of the conflict. Overriding symbols from libc is likely to break in this way as the libc evolves, and since emscripten is now providing a dummy, just as we want, its better/safer to simply remove our dummy.
kripken
added a commit
to WebAssembly/binaryen
that referenced
this pull request
Jan 26, 2022
After emscripten-core/emscripten#15905 lands Emscripten will no longer use it, and nothing else needs it AFAIK.
13 tasks
sbc100
added a commit
that referenced
this pull request
Jun 23, 2022
Runtime callbacks can only be functions, not numbers. In the past we used to handle C atexit using this mechansim which is why this used to be needed but that changd in #15905.
sbc100
added a commit
that referenced
this pull request
Jun 23, 2022
Runtime callbacks can only be functions, not numbers. In the past we used to handle C atexit using this mechansim which is why this used to be needed but that changd in #15905.
sbc100
added a commit
that referenced
this pull request
Jun 23, 2022
Runtime callbacks can only be functions, not numbers. In the past we used to handle C atexit using this mechansim which is why this used to be needed but that changd in #15905.
sbc100
added a commit
that referenced
this pull request
Jun 23, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implementation uses shared memory to store the list of
atexitfunctionsand avoids proxying
atexitcalls back to main thread.This is small step towards a large change to just use musl for all
atexithandling: #14479.The codesize implications of this change are a mixed bag. In some
places we see saving but in other cases the extra export causes a small
regression (only when
EXIT_RUNTIME=1). In the long, once we land #14479there should be more code size saving to be had by doing everything on
the native side.
Fixes #15868